add noise hits to reconstructed hits by randomly generating cell id #1643
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wdconinc
reviewed
Oct 25, 2024
Comment on lines
+110
to
+112
| std::random_device rd; // Obtain a random number from hardware | ||
| std::mt19937_64 eng(rd()); // Seed the generator | ||
| std::uniform_int_distribution<uint64_t> distr; // Define the range for 64-bit integers |
There was a problem hiding this comment.
Do not create a new random device for every hit.
veprbl
reviewed
Oct 26, 2024
Comment on lines
+146
to
+148
| #if EDM4EIC_VERSION_MAJOR >= 7 | ||
| auto rec_hit = | ||
| #endif |
There was a problem hiding this comment.
Suggested change
| #if EDM4EIC_VERSION_MAJOR >= 7 | |
| auto rec_hit = | |
| #endif |
Not needed if you don't need to set a relation.
Comment on lines
+92
to
+96
| //------------------------------------------------ | ||
| // Example code of adding random noise hits | ||
| // Shujie Li, 08.2024 | ||
| // use the first raw hits to obtain the volume ID of the detector system | ||
| // this also make sure the hit container has at least one hit |
There was a problem hiding this comment.
Suggested change
| //------------------------------------------------ | |
| // Example code of adding random noise hits | |
| // Shujie Li, 08.2024 | |
| // use the first raw hits to obtain the volume ID of the detector system | |
| // this also make sure the hit container has at least one hit | |
| // Use the first raw hits to obtain the volume ID of the detector system. | |
| // This also make sure the hit container has at least one hit. |
And add your name on the second line of the file.
| std::random_device rd; // Obtain a random number from hardware | ||
| std::mt19937_64 eng(rd()); // Seed the generator | ||
| std::uniform_int_distribution<uint64_t> distr; // Define the range for 64-bit integers | ||
| dd4hep::Position pos; |
There was a problem hiding this comment.
Suggested change
| dd4hep::Position pos; |
| uint64_t randomNum = distr(eng); // Generate a random 64-bit number | ||
| randomNum = (randomNum & ~uint64_t(0xFF)) | vol_id; // Clear the last 8 bits and set them to 'vol ID' | ||
| try { | ||
| pos = m_converter->position(randomNum); |
There was a problem hiding this comment.
Suggested change
| pos = m_converter->position(randomNum); | |
| auto pos = m_converter->position(randomNum); |
| // std::cout<<"::: cell ID error caught"<<std::endl; | ||
| nn++; | ||
| } | ||
| std::cout <<std::bitset<8>(vol_id) <<":"<< i<<"/"<<nn<<":::"<<randomNum<<" "; |
There was a problem hiding this comment.
Please, remove "cout" output (even commented), or replace with clearly worded debug logging.
Comment on lines
+114
to
+115
| int nn=0; | ||
| int i=0; |
There was a problem hiding this comment.
Please give clear name to the variables.
Suggested change
| int nn=0; | |
| int i=0; | |
| unsigned int num_failures = 0; | |
| unsigned int num_generated_hits = 0; |
Comment on lines
+102
to
+106
| uint64_t id0,vol_id; | ||
| id0 = hit0.getCellID(); | ||
| // vol ID is the last 8 bits in Si tracker. Need to make it more flexible | ||
| // may want to predefine the layer/module ID as well to speed up the radom ID generation | ||
| vol_id = id0 & 0xFF; |
There was a problem hiding this comment.
Should we not call it "system ID"?
Suggested change
| uint64_t id0,vol_id; | |
| id0 = hit0.getCellID(); | |
| // vol ID is the last 8 bits in Si tracker. Need to make it more flexible | |
| // may want to predefine the layer/module ID as well to speed up the radom ID generation | |
| vol_id = id0 & 0xFF; | |
| uint64_t id0, system_id; | |
| id0 = hit0.getCellID(); | |
| // system ID is the last 8 bits in Si tracker. Need to make it more flexible | |
| // may want to predefine the layer/module ID as well to speed up the radom ID generation | |
| system_id = id0 & 0xFF; |
veprbl
reviewed
Oct 26, 2024
Comment on lines
+117
to
+118
| uint64_t randomNum = distr(eng); // Generate a random 64-bit number | ||
| randomNum = (randomNum & ~uint64_t(0xFF)) | vol_id; // Clear the last 8 bits and set them to 'vol ID' |
There was a problem hiding this comment.
I like how this is clear and simple, but is performance terrible or not?
simonge
reviewed
Oct 28, 2024
Comment on lines
+95
to
+96
| // use the first raw hits to obtain the volume ID of the detector system | ||
| // this also make sure the hit container has at least one hit |
There was a problem hiding this comment.
Is this something that you really want or just a consequence of how you're getting the volume? Might it not be better to have a completely separate algorithm which constructs these hits from knowledge of the readout structure to get the volume rather than relying on their being a hit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Briefly, what does this PR introduce?
add noise hits to reconstructed hits by randomly generating cell id.
for test only. The generated cell ID is not always on sensitive surface.
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
Does this PR change default behavior?